This is essentially an alternative to #8537, right? At least the Status part of that.

As already mentioned there, we should also consider #8647, which essentially makes part of the special handling unneccessary. So I'd very much prefer to remove exit_status first (or in the same go).

It's not an alternative, more like a subset.

Deprecating that method is just that, an added annotation. It does not remove any need for that special handling.

Yeah, you're right. Let me rephrase: It would have been better to deprecate and remove exit_status first 😆

Hm no, it really makes no difference. The two PRs support each other well (i.e. merging either one makes the other one make even more sense) but don't depend on each other in any way at all (other than needing a manual merge)

BTW there's no backwards-incompatible change here

Adding more if flag? outside of src/crystal/system is not the way I want to go about this. A struct containing the actual parsed data would be preferred, then make the Crystal::System::Process do the signal/exit code extraction preemptively and return the struct.

I can agree with that only on the matter of principle. But I definitely think for this particular case keeping it in one file is better for maintenance.

@RX14 As per #8647 (comment) I'd remove those platform-specifics from Process::Status altogether.

Me too.

Just, record Process::Status with exit_signal : Signal? and exit_code : Int32 and re-add the query helpers is fine.

I can grant this wish by copying the implementations of reader methods to the place where the struct is constructed, without having any idea how this exit status is processed.
I was thinking that perhaps this could be a dumb struct, and the implementations could just be put into crystal/system/process, but I don't see how to nicely solve the requirement that calling exit_signal needs to raise on Windows.

I don't see how to nicely solve the requirement that calling exit_signal needs to raise on Windows.

The problem is that the method returns Signal, even when the program didn't signal exit on posix either. It'd be much saner to have it return nil if the program didn't exit with a signal, be it either on windows or unix.

Or just don't have exit_signal at all...

Yes I'm very tempted to implement it as such...
Also curious to explore the possibility of making it just a number like many other programming languages do.

The discussion is sadly spread so much,
not much on the issue #8381
but quite some discussion on PR #8647

I'll add by comparing Crystal to what Python does. For my own understanding, too.

The outcome is that Python just returns a negative number if it's a signal.

$ crystal eval 'require "process"; r = Process.run("cat", input: :inherit); p! r.exit_status, r.exit_code, r.exit_signal' & while ! killall cat; do sleep 0.5; done; fg 
r.exit_status # => 15
r.exit_code   # => 0
r.exit_signal # => TERM

$ python3 -c 'import subprocess; print(subprocess.call("cat"))' & while ! killall cat; do sleep 0.5; done; fg
-15

$ crystal eval 'require "process"; r=Process.run("false", input: :inherit); p! r.exit_status, r.exit_code, r.exit_signal'                                              
r.exit_status # => 256
r.exit_code   # => 1
r.exit_signal # => Unhandled exception: Unknown enum Signal value: 0 (Exception)

$ python3 -c 'import subprocess; print(subprocess.call("false"))'                                              
1

It also seems quite strange to me that Crystal enforces that you can't find out the signal if it's one that Crystal is not aware of.

Also returning exit_code as 0 when the process is terminated.....

And coming back to the "dumb struct", it could turn out quite inefficient in terms of storage, perhaps as far as this:
record exit_code : Int64?, exit_signal : Int64?
- what is it now - 24 bytes? 32?
when really 4 bytes is all that's needed

Honestly I'm not so interested in solving the design issue right here.

I think that the current state of the pull request is the only way to do it without hitting any of the disadvantages that other approaches here are prone to:

• backwards incompatible
• inefficient
• blocked on other changes

If one wants to follow this up with a more idealistic approach, this doesn't make it harder to do.

Please read this, I summarized all possible status codes:
#8381 (comment)

Having looked into how diverse the exit status can be, I don't see it as reasonable to pre-process and store this data in a plain struct that is the same on all platforms.

I am asking to merge this as is. There are no changes in behavior on supported systems. The only complaints are about where the code should ideally be. But it's clear that this will need more thought, and we will be in a better position for maneuvers after Process support on Windows is landed (as it will actually make the file crystal/system/process exist, which is where you want me to put this). But that change is blocked on this one.

cc @asterite

Thanks @oprypin

#9064 will be my only followup to this. It's the only way to meet the above requirements.

The suggestions for refactoring here insisted on storing/exposing only processed data and throwing out exit_status. In that case we have to ensure that the processed data provides the full richness of information, otherwise some things become impossible to do (not conveniently do, but at all), and that's just a downgrade from the current state.